fix(datasets): require pipeline options instead of substituting wrong defaults - #1413
Open
Maryyyyyyyam142 wants to merge 1 commit into
Open
Conversation
… defaults The dataset builders read the same image-pipeline options from `args`, but `build_roboflow_from_coco`, `build_roboflow_from_yolo` and `build_o365_raw` read them via `getattr` with literal fallbacks that contradict the real config defaults, while `build_coco` reads most of them directly: square_resize_div_64 fallback False, TrainConfig default True segmentation_head fallback False, True on every seg variant multi_scale fallback False, TrainConfig default True expanded_scales fallback False, TrainConfig default True patch_size fallback 16, variant-dependent (12/14/16) num_windows fallback 4, 2 on every released variant `patch_size` and `num_windows` have no default on `ModelConfig`, so no constant is correct; the pair the builders chose, (16, 4), matches no shipped variant. A caller passing an incomplete namespace — supported and documented usage, since the builders are re-exported from `rfdetr.datasets` — silently trained a different pipeline: multi-scale off, and a scale set of [192..832] instead of [352..672] at resolution 512. Read these seven options directly in all four builders so an incomplete namespace fails loudly. The remaining `getattr` fallbacks are left alone: their values match the config default, or the absence is meaningful (keypoint fields absent means detection-only, per the comment at coco.py). This is latent on the `RFDETRDataModule` path, where `_namespace_from_configs` populates every field.
Maryyyyyyyam142
requested review from
Borda,
SkalskiP,
isaacrob and
probicheaux
as code owners
August 31, 2026 12:53
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The contract tests only verify the first missing option and do not protect the remaining required fields.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Requires explicit dataset pipeline options to prevent silent, incorrect defaults in direct builder calls.
Changes:
- Replaces fallback values with direct namespace access.
- Adds cross-builder contract tests.
- Updates existing tests with explicit pipeline arguments.
File summaries
| File | Description |
|---|---|
src/rfdetr/datasets/coco.py |
Requires configured COCO pipeline options. |
src/rfdetr/datasets/yolo.py |
Requires configured YOLO pipeline options. |
src/rfdetr/datasets/o365.py |
Requires the square-resize option. |
tests/datasets/test_builder_options.py |
Adds builder option contract tests. |
tests/datasets/test_coco.py |
Supplies explicit options in existing tests. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 3
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| from rfdetr.datasets.yolo import build_roboflow_from_yolo | ||
|
|
||
| # The options that must come from args, with no literal fallback. Ordered as the builders read them. | ||
| REQUIRED_PIPELINE_OPTIONS = ( |
| # Read directly rather than via getattr with a literal default: these options have no safe constant. | ||
| # patch_size, num_windows and segmentation_head are variant-dependent, and square_resize_div_64, | ||
| # multi_scale and expanded_scales all default to True on TrainConfig. An incomplete namespace must fail | ||
| # here instead of silently building a different pipeline (GitHub #N). The optional fields below keep getattr on |
| """A namespace missing a pipeline option must raise, not train a silently different pipeline.""" | ||
|
|
||
| @pytest.fixture | ||
| def partial_namespace(self, tmp_path) -> types.SimpleNamespace: |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #1413 +/- ##
=======================================
Coverage 86% 86%
=======================================
Files 114 114
Lines 14880 14880
=======================================
Hits 12835 12835
Misses 2045 2045 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1412
The four dataset builders derive the same image-pipeline options from
args, but disagree on how.build_cocoreads most of them directly;build_roboflow_from_coco,build_roboflow_from_yoloandbuild_o365_rawusegetattrwith literal fallbacks — five of which contradict the real config default:square_resize_div_64FalseTrue(TrainConfig)segmentation_headFalseTrueon every seg variantmulti_scaleFalseTrue(TrainConfig)expanded_scalesFalseTrue(TrainConfig)patch_size1612seg/keypoint,14basenum_windows42on every released variantpatch_sizeandnum_windowshave no default onModelConfig, so no constant is correct — and(16, 4)matches no shipped variant.Change
All four builders now read these seven options directly, so an incomplete namespace raises
AttributeErrorinstead of silently building a different pipeline.do_random_resize_via_paddingis included for cluster parity; its fallback already matched the default, so that one is a no-op.The remaining
getattrfallbacks are deliberately untouched —aug_config,scale_jitterandaugmentation_backendmatch their config default, and the keypoint fields' absence is meaningful ("detection-only", per the existing comment atcoco.py:1301).Scope
Latent on the
RFDETRDataModulepath:_namespace_from_configspopulates every field, so normalmodel.train(...)is unaffected. The exposure is the direct-call path, which the builders' re-export fromrfdetr.datasetsand the comment atcoco.py:1318-1322both document as supported.Tests
New
tests/datasets/test_builder_options.pycovers the cross-builder contract:develop)num_windows=2not4,multi_scale=True, seg variants atpatch_size=12withinclude_masks=TrueFour tests in
test_coco.pypassed partial namespaces and relied on the old fallbacks. They now use a_pipeline_argshelper that spells out the values those fallbacks used to produce, so their behavior is unchanged and what they depend on is visible.674 passedacrosstests/datasets/andsrc/rfdetr/datasets/.ruff,ruff format,docformatterandcodespellpass on the changed files;mypyreports nothing in them.